Skip to content

fix(web): guard undefined hash in commit detail view - #7148

Merged
khanhtc1202 merged 1 commit into
pipe-cd:masterfrom
vikash7485:fix/guard-undefined-commit-hash
Aug 22, 2026
Merged

fix(web): guard undefined hash in commit detail view#7148
khanhtc1202 merged 1 commit into
pipe-cd:masterfrom
vikash7485:fix/guard-undefined-commit-hash

Conversation

@vikash7485

@vikash7485 vikash7485 commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

What this PR does:

Guard against undefined hash in the deployment detail commit section by adding a null-coalescing fallback (?? "") before calling .slice(0, 7).

Why we need it:

When deployment.trigger.commit.hash is undefined (possible from older Piped v0 protobuf messages where the field was unpopulated), the current code calls .slice(0, 7) directly on undefined, causing a runtime crash:

TypeError: Cannot read properties of undefined (reading 'slice')

The outer guard at L254 (deployment.trigger?.commit &&) only checks that commit exists, not that hash is populated. This fix adds a safe fallback so the link renders with an empty string instead of crashing.

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:

No. This prevents a crash for an edge case. Users with valid commit hashes see no difference.

  • How are users affected by this change: Deployments with missing commit hashes no longer crash the detail page.
    • Is this breaking change: No.
    • How to migrate (if breaking change): N/A.

@vikash7485
vikash7485 requested review from a team as code owners August 11, 2026 03:50
@vikash7485
vikash7485 force-pushed the fix/guard-undefined-commit-hash branch from 9fc67c1 to 818473d Compare August 11, 2026 04:02
rahulshendre
rahulshendre previously approved these changes Aug 21, 2026
View reviewed changes

@rahulshendre rahulshendre left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGMT, thanks @vikash7485

one thing I am curious about - commit.hash is typed string from the protobuf (proto3 defaults an unset string to "", not undefined), so I couldn't see where it'd actually be undefined.
If you saw it, please let use know where, mostly so we know if message/url next to it need the same

@vikash7485

vikash7485 commented Aug 21, 2026

Copy link
Copy Markdown
Contributor Author

@rahulshendre Thanks for the review and approval

The reason hash was specifically at risk here is that it had a method call (hash.slice(0, 7)), whereas message and url are passed directly into JSX (<Typography> and href), which React safely handles even if they are undefined.

In standard protobuf generated classes, proto3 does default strings to "". However, in frontend tests/mocks, partial payloads, or when objects are deserialized from JSON without emitDefaults: true, omitted properties can end up as undefined in TypeScript. (hash ?? "").slice(0, 7) ensures that even in those edge cases, the component renders safely without throwing a runtime TypeError.

@netlify

netlify Bot commented Aug 22, 2026 •
edited
Loading

Copy link
Copy Markdown

✅ Deploy Preview for pipecd-site canceled.

Name Link
🔨 Latest commit e7b4c61
🔍 Latest deploy log https://app.netlify.com/projects/pipecd-site/deploys/6a898331b61dfe0008c8a1a3

khanhtc1202
khanhtc1202 previously approved these changes Aug 22, 2026
View reviewed changes

@khanhtc1202 khanhtc1202 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch, thank you 👍

@khanhtc1202
khanhtc1202 enabled auto-merge (squash) August 22, 2026 08:22
@codecov

codecov Bot commented Aug 22, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 29.01%. Comparing base (4bb2366) to head (3a04558).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #7148      +/-   ##
==========================================
- Coverage   29.68%   29.01%   -0.68%     
==========================================
  Files         601      532      -69     
  Lines       64383    58157    -6226     
==========================================
- Hits        19113    16873    -2240     
+ Misses      43779    39919    -3860     
+ Partials     1491     1365     -126
Flag Coverage Δ
. 23.58% <ø> (ø)
.-pkg-app-pipedv1-plugin-analysis ?
.-pkg-app-pipedv1-plugin-ecs 31.74% <ø> (ø)
.-pkg-app-pipedv1-plugin-kubernetes 58.39% <ø> (ø)
.-pkg-app-pipedv1-plugin-kubernetes_multicluster 59.62% <ø> (ø)
.-pkg-app-pipedv1-plugin-scriptrun 54.83% <ø> (ø)
.-pkg-app-pipedv1-plugin-terraform ?
.-pkg-app-pipedv1-plugin-wait ?
.-pkg-app-pipedv1-plugin-waitapproval 52.71% <ø> (ø)
.-pkg-plugin-sdk ?
.-tool-actions-gh-release ?
.-tool-actions-plan-preview ?
.-tool-codegen-protoc-gen-auth ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@khanhtc1202

khanhtc1202 commented Aug 22, 2026

Copy link
Copy Markdown
Member

@vikash7485 same here, need to run make lint/web on your local to fix this

auto-merge was automatically disabled August 22, 2026 10:53

Head branch was pushed to by a user without write access

@vikash7485
vikash7485 dismissed stale reviews from khanhtc1202 and rahulshendre via 1390e3b August 22, 2026 10:53
@khanhtc1202

khanhtc1202 commented Aug 22, 2026

Copy link
Copy Markdown
Member

@vikash7485 the DCO check has failed 👀 You need to sign-off the commit 😁

@vikash7485
fix(web): guard undefined hash in commit detail view and add regressi…
e7b4c61 
…on test

Signed-off-by: vikash7485 <vikkiraj073@gmail.com>
@vikash7485
vikash7485 force-pushed the fix/guard-undefined-commit-hash branch from e4d49c4 to e7b4c61 Compare August 22, 2026 11:08
@khanhtc1202
khanhtc1202 merged commit 7e22859 into pipe-cd:master Aug 22, 2026
64 checks passed
@github-actions

github-actions Bot commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

Thank you for contributing to PipeCD, @vikash7485! The changes in this pull request will be part of the upcoming release!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@khanhtc1202 khanhtc1202 khanhtc1202 approved these changes
@hongky-1994 hongky-1994 Awaiting requested review from hongky-1994 hongky-1994 is a code owner automatically assigned from pipe-cd/pipecd-approvers-web
@Warashi Warashi Awaiting requested review from Warashi Warashi is a code owner automatically assigned from pipe-cd/pipecd-approvers
@t-kikuc t-kikuc Awaiting requested review from t-kikuc t-kikuc is a code owner automatically assigned from pipe-cd/pipecd-approvers
@rahulshendre rahulshendre Awaiting requested review from rahulshendre

Assignees

No one assigned

Labels

area/web

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@vikash7485 @khanhtc1202 @rahulshendre